-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for static logger member in abstract class 'SQLServerClobBase' #537
Fix for static logger member in abstract class 'SQLServerClobBase' #537
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #537 +/- ##
============================================
- Coverage 46.68% 46.62% -0.06%
+ Complexity 2228 2215 -13
============================================
Files 108 108
Lines 25308 25306 -2
Branches 4175 4175
============================================
- Hits 11815 11799 -16
- Misses 11463 11476 +13
- Partials 2030 2031 +1
Continue to review full report at Codecov.
|
@@ -91,7 +88,9 @@ final JDBCType getJdbcType() { | |||
private ArrayList<Closeable> activeStreams = new ArrayList<>(1); | |||
|
|||
transient SQLServerConnection con; | |||
private static Logger logger; | |||
|
|||
private final Logger logger = Logger.getLogger(getClass().getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this comment that got deleted?
// Loggers should be class static to avoid lock contention with multiple threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well - its because this logger object won't be static in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test result looks good.
@@ -91,7 +88,9 @@ final JDBCType getJdbcType() { | |||
private ArrayList<Closeable> activeStreams = new ArrayList<>(1); | |||
|
|||
transient SQLServerConnection con; | |||
private static Logger logger; | |||
|
|||
private final Logger logger = Logger.getLogger(getClass().getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now a logger lookup whenever an instance of SQLServerClob
or SQLServerNClob
is created. This could be avoided if:
- there is a
static final
logger inSQLServerClob
orSQLServerNClob
, like before - the logger in
SQLServerClobBase
is madeprivate final
, like in this PR SQLServerClob
andSQLServerNClob
pass the logger in thesuper
constructor call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marschall
Thanks for replying back! We could go for either solution, but I removed the loggers from sub classes as:
- Class Lookup isn't expensive here.
- The loggers in sub classes are not used anywhere else except being passed in constructor (but remain attached in memory with class objects)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the class lookup that I was concerned about, it's get logger lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain what you mean by Logger lookup and how will it be different in the other case - maybe I am not able to understand your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a non-static logger in SQLServerClob
or SQLServerNClob
every time a new instance of these classes is generated the following is executed:
Logger.getLogger(getClass().getName())
If we ignore getClass().getName()
for now that still leaves us with Logger.getLogger()
which does among other things:
Reflection.getCallerClass()
which creates an exception and walks the entire stack- check for a security manager several times
- goes through a global lock in
java.util.logging.LogManager.LoggerContext#namedLoggers
If you make the loggers in SQLServerClob
and SQLServerNClob
static
all that work happens only one during class loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marschall I understand the concern for non-static logger here. Made changes in the new PR #563 as discussed above. Request you to review the PR so we can get it working.
Fixes issue #186.
This change makes logger in abstract class 'SQLServerClobBase' non-static but final such that it is instantiated for every new object, based on subclass been used to create object and is immutable.